Add `task_acks_late` configuration to Celery Executor #37066

ninsbl · 2024-01-29T12:15:54Z

Celery task_acks_late is currently hard-coded to be deactivated. However, this causes issues when tasks exceed the visibility_timeout.

This PR, adds a task_acks_late parameter to the Celery configuration. Default values remain unchanged, but with the new config options users can at least try, if this setting solves there issue if they encounter the 'Task Instance Not Running' FAILED: Task is in the running state error message.

It is an enhanced re-implementation of #31829 and adresses #16163.

closes #16163
related: #31829

This is not covered by tests (yet). I can add tests but would be happy to receive pointers to e.g. existing test for environment settings, that I could adjust.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

dirrao

You can through the following page for writing and running the tests.
https://github.com/apache/airflow/blob/main/contributing-docs/09_testing.rst

hussein-awala

Could you add a test?

airflow/providers/celery/executors/default_celery.py

airflow/providers/celery/provider.yaml

hussein-awala · 2024-01-30T18:28:13Z

I just checked the conversation in the original issue, and personally I prefer to keep the default value to True and waits for community feedback before deciding if we need to change it to False. Let's wait for others reviews.

ninsbl · 2024-01-30T19:00:53Z

I just checked the conversation in the original issue, and personally I prefer to keep the default value to True and waits for community feedback before deciding if we need to change it to False. Let's wait for others reviews.

Sure, False as default was a glitch. It makes a lot of sense to keep the current default.

I am happy to add tests. What are optional and what are required tests? E.g. I have no experience with Kubernetes and Helm charts. So owuld for example be a unit test that checks if the variable is set properly be enough? With the Kubernetes stuff I may need assistance to get that merge-ready soonish...

hussein-awala · 2024-01-30T20:24:43Z

I am happy to add tests. What are optional and what are required tests? E.g. I have no experience with Kubernetes and Helm charts. So owuld for example be a unit test that checks if the variable is set properly be enough? With the Kubernetes stuff I may need assistance to get that merge-ready soonish...

You can add a small test to check if the conf is taken into account by Airflow, you can use this test as a reference:

airflow/tests/providers/celery/executors/test_celery_executor.py

Lines 352 to 360 in e2393ee

    
           @conf_vars({("celery_broker_transport_options", "sentinel_kwargs"): '{"service_name": "mymaster"}'}) 
        
           def test_sentinel_kwargs_loaded_from_string(): 
        
               import importlib 
        
               # reload celery conf to apply the new config 
        
               importlib.reload(default_celery) 
        
               assert default_celery.DEFAULT_CELERY_CONFIG["broker_transport_options"]["sentinel_kwargs"] == { 
        
                   "service_name": "mymaster" 
        
               }

ninsbl · 2024-01-30T22:56:09Z

Tried to add a basic unittest in b9be045. Checked it with breeze and it seems to work...

tests/providers/celery/executors/test_celery_executor.py

hussein-awala

LGTM

airflow/providers/celery/provider.yaml

ninsbl · 2024-01-31T08:05:11Z

BTW.: When running tests with breeze (on WSL2) I get the following error message:

23.16 INFO: pip is looking at multiple versions of apache-airflow[amazon] to determine which version is compatible with other requirements. This could take a while.
23.31 ERROR: Cannot install apache-airflow[amazon]==2.9.0.dev0 because these package versions have conflicting dependencies.
23.31
23.31 The conflict is caused by:
23.31     apache-airflow[amazon] 2.9.0.dev0 depends on moto<5.0.0 and >=4.2.12; extra == "amazon"
23.31     The user requested (constraint) moto==5.0.0
23.31
23.31 To fix this you could try to:
23.31 1. loosen the range of package versions you've specified
23.31 2. remove package versions to allow pip attempt to solve the dependency conflict
23.31
23.31 ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts
------
Dockerfile.ci:1190
--------------------
 1188 |     # But in cron job we will install latest versions matching pyproject.toml to see if there is no breaking change
 1189 |     # and push the constraints if everything is successful
 1190 | >>> RUN bash /scripts/docker/install_airflow.sh
 1191 |
 1192 |     COPY --from=scripts entrypoint_ci.sh /entrypoint
--------------------
ERROR: failed to solve: process "/bin/bash -o pipefail -o errexit -o nounset -o nolog -c bash /scripts/docker/install_airflow.sh" did not complete successfully: exit code: 1

This does not prevent unittests from being executed though...

potiuk · 2024-01-31T08:50:00Z

BTW.: When running tests with breeze (on WSL2) I get the following error message:

This does not prevent unittests from being executed though...

Yes. Yu need to rebase and rerun it to fix it. Breeze /tests works for sure in latest main. Alternatively (but this is something breeze will always print as warning in this case) there are times before merging conflicting changes to main and refreshing constraints where you need to run build with --upgrade-to-newer-dependencies.

Actually I think I wil make a small change to handle this case better because we already have feature to retry build with --upgrade-to-newer-dependencies when it fails with constraints , so I can flip the flag to run this mode locally by default

Co-authored-by: Hussein Awala <[email protected]>

Co-authored-by: Niko Oliveira <[email protected]>

potiuk · 2024-01-31T09:06:35Z

I rebased this one and added PR to make future local build work in case we have not managed to refresh the constraints yet #37116 - that will take longer but will finally succeed and should close the loophole here.

ninsbl · 2024-01-31T09:37:00Z

Yes. Yu need to rebase and rerun it to fix it. Breeze /tests works for sure in latest main.

Thanks, @potiuk , I can confirm that breeze now builds and runs fine after rebase...

ninsbl · 2024-02-01T21:42:05Z

Anything I can do to move this forward?

ninsbl · 2024-02-01T22:45:42Z

Hmm, it seems the spell checking does ot like the new key name:

(Incorrect Spelling: 'acks' Line with Error: 'task_acks_late')...

ninsbl · 2024-02-03T10:36:15Z

Hope adding the exception for acks in 8419527 was correct resolution for the failing spell check test ... ?

airflow/providers/celery/provider.yaml

Co-authored-by: Jarek Potiuk <[email protected]>

boring-cyborg · 2024-02-05T23:07:54Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

hussein-awala · 2024-02-05T23:08:17Z

Congrats on your first commit 🎉

ninsbl requested a review from hussein-awala as a code owner January 29, 2024 12:15

boring-cyborg bot added area:providers provider:celery labels Jan 29, 2024

potiuk mentioned this pull request Jan 29, 2024

Celery worker enters a catatonic state after redis restart #26542

Open

2 tasks

dirrao requested changes Jan 30, 2024

View reviewed changes

hussein-awala reviewed Jan 30, 2024

View reviewed changes

airflow/providers/celery/executors/default_celery.py Outdated Show resolved Hide resolved

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

hussein-awala reviewed Jan 30, 2024

View reviewed changes

tests/providers/celery/executors/test_celery_executor.py Outdated Show resolved Hide resolved

hussein-awala approved these changes Jan 31, 2024

View reviewed changes

hussein-awala requested review from potiuk, kaxil, dstandish and eladkal January 31, 2024 00:10

o-nikolas reviewed Jan 31, 2024

View reviewed changes

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

ninsbl and others added 9 commits January 31, 2024 10:04

get task_acks_late from config

2afd2a7

add task_acks_late to config

b91bc43

Update airflow/providers/celery/provider.yaml

00228e3

Co-authored-by: Hussein Awala <[email protected]>

Update airflow/providers/celery/executors/default_celery.py

ead016d

Co-authored-by: Hussein Awala <[email protected]>

add basic test

a7ee86f

test acks_late False

a7592eb

linting

bd57b96

Update airflow/providers/celery/provider.yaml

e079550

Co-authored-by: Niko Oliveira <[email protected]>

Update airflow/providers/celery/provider.yaml

1d9aa5e

Co-authored-by: Niko Oliveira <[email protected]>

potiuk force-pushed the celery_task_acks_late branch from 50420ee to 1d9aa5e Compare January 31, 2024 09:04

linting

395f667

Update spelling_wordlist.txt

8419527

ninsbl added 2 commits February 3, 2024 12:26

alphabetic order

15fe259

Merge branch 'main' into celery_task_acks_late

b920a2b

ninsbl requested review from dirrao and o-nikolas February 3, 2024 11:29

ninsbl added 2 commits February 5, 2024 22:26

define version added

b3bf903

Merge branch 'main' into celery_task_acks_late

ac8d023

potiuk approved these changes Feb 5, 2024

View reviewed changes

hussein-awala approved these changes Feb 5, 2024

View reviewed changes

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

Update airflow/providers/celery/provider.yaml

6cb1a13

potiuk reviewed Feb 5, 2024

View reviewed changes

airflow/providers/celery/provider.yaml Outdated Show resolved Hide resolved

Update airflow/providers/celery/provider.yaml

9be5a5e

Co-authored-by: Jarek Potiuk <[email protected]>

hussein-awala changed the title ~~Celery provider: Add task_acks_late setting~~ Add task_acks_late configuration to Celery Executor Feb 5, 2024

hussein-awala merged commit 6c72223 into apache:main Feb 5, 2024
56 checks passed

eladkal mentioned this pull request Feb 12, 2024

Status of testing Providers that were prepared on February 12, 2024 #37358

Closed

81 tasks

hussein-awala mentioned this pull request Mar 19, 2024

Disable celery task_acks_late #31829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `task_acks_late` configuration to Celery Executor #37066

Add `task_acks_late` configuration to Celery Executor #37066

ninsbl commented Jan 29, 2024

dirrao left a comment

hussein-awala left a comment

hussein-awala commented Jan 30, 2024

ninsbl commented Jan 30, 2024

hussein-awala commented Jan 30, 2024

ninsbl commented Jan 30, 2024

hussein-awala left a comment

ninsbl commented Jan 31, 2024

potiuk commented Jan 31, 2024

potiuk commented Jan 31, 2024

ninsbl commented Jan 31, 2024

ninsbl commented Feb 1, 2024

ninsbl commented Feb 1, 2024

ninsbl commented Feb 3, 2024

boring-cyborg bot commented Feb 5, 2024

hussein-awala commented Feb 5, 2024

Add task_acks_late configuration to Celery Executor #37066

Add task_acks_late configuration to Celery Executor #37066

Conversation

ninsbl commented Jan 29, 2024

dirrao left a comment

Choose a reason for hiding this comment

hussein-awala left a comment

Choose a reason for hiding this comment

hussein-awala commented Jan 30, 2024

ninsbl commented Jan 30, 2024

hussein-awala commented Jan 30, 2024

ninsbl commented Jan 30, 2024

hussein-awala left a comment

Choose a reason for hiding this comment

ninsbl commented Jan 31, 2024

potiuk commented Jan 31, 2024

potiuk commented Jan 31, 2024

ninsbl commented Jan 31, 2024

ninsbl commented Feb 1, 2024

ninsbl commented Feb 1, 2024

ninsbl commented Feb 3, 2024

boring-cyborg bot commented Feb 5, 2024

hussein-awala commented Feb 5, 2024

Add `task_acks_late` configuration to Celery Executor #37066

Add `task_acks_late` configuration to Celery Executor #37066